runner: measure benchmark memory and CPU from the process subtree by adriangb · Pull Request #14 · adriangb/datafusion-benchmarking

adriangb · 2026-06-09T14:30:01Z

Problem

The resource monitor reads the pod-wide cgroup (/sys/fs/cgroup/memory.current and cpu.stat) over the whole monitored window. For benchmarks whose bench.sh run step compiles (the Criterion SQL harness — cargo bench --bench sql), that window includes the compiler, so the reported "Peak memory" and "CPU" are dominated by rustc/cargo, not the benchmark.

Observed live on a wide_schema run: the comment reported Peak memory 62.2 GiB and CPU user ~2942 s — that's the parallel build + LTO compile, not the queries (which use ~1 GB). The numbers are effectively measuring the toolchain.

Fix

Scope both memory and CPU to the spawned benchmark command's process subtree, excluding compiler/build-tool processes (rustc, cargo, cc, ld, sccache, build-script-*, …):

memory: sum VmRSS across the subtree each second (peak/avg) by walking /proc.
CPU: accumulate per-process utime/stime deltas over the window, so process churn (the bench binary starting after the compile finishes) is handled and the compile's CPU is excluded.

shell.rs is refactored to split spawn from wait so the monitor receives the child PID. When no PID is available — or /proc is absent (e.g. local macOS) — it falls back to the pod-wide cgroup figures as before, so behavior is unchanged off-cluster.

Background

This change was written earlier but never landed: it was pushed to the builds-no-debuginfo-by-default branch after that PR (#11) had already been merged at the debuginfo-only commit, so only the debuginfo change reached main. This PR brings just the metric-scoping change in, cleanly on top of current main.

Testing

cargo fmt --check, cargo clippy --all-targets -- -D warnings, and cargo test (138 tests, incl. unit tests for the /proc parsing, build-tool classification, and a self-subtree memory/CPU smoke test) all pass.

🤖 Generated with Claude Code

The resource monitor read the pod-wide cgroup (`memory.current` / `cpu.stat`), so the reported "Peak memory" and "CPU" for a benchmark also counted page cache, leftover build artifacts, and — for benchmarks whose `bench.sh run` step compiles (e.g. `cargo bench --bench sql`) — the compiler itself. On a live wide_schema run that meant a single rustc (~16 GB, 12 cores) was being attributed to the benchmark. Scope both memory and CPU to the spawned benchmark command's process subtree by walking /proc, excluding compiler/build-tool processes (rustc, cargo, cc, ld, sccache, build-script-*, …): - memory: sum VmRSS over the subtree each second (peak/avg). - CPU: accumulate per-process utime/stime deltas over the window, so process churn (the bench binary starting after the compile finishes) is handled and the compile's CPU is excluded. shell.rs splits spawn from wait so the monitor gets the child pid. When no pid is available (or no /proc, e.g. local macOS) it falls back to the cgroup figures as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The self-subtree test ran on a single-threaded tokio runtime and spun for 1.2s, so the background poll task was starved (and CPU is a delta needing >=2 samples at the 1s poll interval) — it observed zero CPU and failed on Linux. Use a multi-thread runtime, spin >2s so at least two polls land, and guard the CPU assertion on having enough samples. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR refines benchmark resource reporting by scoping memory/CPU measurement to the spawned benchmark command’s process subtree, avoiding inflated numbers caused by compiler/toolchain activity during bench.sh run (e.g., cargo bench compilation). It also refactors command execution helpers so the resource monitor can observe the spawned child PID while the command runs.

Changes:

Refactor shell.rs to split “spawn with logging” from “wait + collect logs”, enabling PID-aware monitoring.
Update the resource monitor to sample RSS and CPU from /proc for the benchmark’s process subtree (excluding build-tool processes), with intended fallback behavior to cgroup stats when needed.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
controller/src/runner/shell.rs	Refactors command execution to expose the spawned child PID to the monitor while preserving existing logging/collection behavior.
controller/src/runner/monitor.rs	Implements `/proc`-based subtree sampling (RSS + CPU) and updates the monitor’s accounting logic and tests accordingly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+//! CPU stats still come from cgroup v2 `cpu.stat`. When the per-process root
+//! pid is unknown the sampler falls back to the cgroup memory figure. When
+//! neither `/proc` nor the cgroup files are available (e.g. local macOS
+//! development), reads return `None` and stats default to zero.


+                    } else if root_pid.is_none() {
+                        // Fallback path: keep memory sampling via cgroup.
+                        if let Some(current) = read_memory_current() {
+                            peak.fetch_max(current, Ordering::Relaxed);
+                            sum.fetch_add(current, Ordering::Relaxed);
+                            count.fetch_add(1, Ordering::Relaxed);
+                        }


+        // CPU: per-process subtree accumulation when we have a root pid (so the
+        // compile is excluded), otherwise the pod-wide cgroup delta.
+        let (cpu_user, cpu_sys) = if self.root_pid.is_some() {
+            (
+                self.cpu_user_usec.load(Ordering::Relaxed),
+                self.cpu_sys_usec.load(Ordering::Relaxed),
+            )


+/// Clock ticks (USER_HZ) to microseconds. Linux USER_HZ is 100 on all common
+/// configurations, so a tick is 10 ms = 10_000 µs.
+fn ticks_to_usec(ticks: u64) -> u64 {
+    ticks.saturating_mul(10_000)
+}


Address review feedback on #14: - Fall back to the pod-wide cgroup figures whenever `/proc` subtree sampling fails, not only when the root pid is unknown. A `proc_ok` flag tracks whether any `/proc` sample succeeded; memory and CPU both fall back to cgroup when it stays false (root pid unknown, or `/proc` unreadable in a restricted container). `sample_memory(Some(pid))` now also falls back to `memory.current` instead of returning `None`/0. - Take a final memory+CPU sample once stop is signalled, so CPU is captured up to the end of the run rather than as of the last full poll interval (was undercounting by up to POLL_INTERVAL with no final /proc read). - Read `_SC_CLK_TCK` via `libc::sysconf` (cached) instead of hard-coding USER_HZ=100, so CPU time is correct on non-100Hz kernels. - Update module docs to describe the subtree-or-cgroup behavior for both memory and CPU (CPU no longer "still comes from cgroup cpu.stat"). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

adriangb · 2026-06-09T15:03:27Z

Addressed the review feedback in 3407d2e:

Module docs — rewritten to describe the subtree-or-cgroup behavior for both memory and CPU; no longer claims CPU "still comes from cgroup cpu.stat".
/proc unreadable with a known pid — the poll loop now falls back to cgroup memory whenever proc_tree_sample returns None (not only when root_pid is None). A new proc_ok flag records whether any /proc sample succeeded.
sample_memory(Some(pid)) — now .or_else(read_memory_current), so it falls back to memory.current instead of returning None/0 when /proc is absent.
CPU undercount — a final memory+CPU sample is taken once stop is signalled, so CPU is captured up to the end of the run rather than as of the last full poll interval. (Also removed the now-redundant end-sample double-count in finish.)
USER_HZ assumption — ticks_to_usec now divides by sysconf(_SC_CLK_TCK) (cached via OnceLock, falling back to 100) instead of hard-coding 10ms ticks.

cargo fmt --check, cargo clippy --all-targets -- -D warnings, and cargo test (138 tests) all pass.

adriangb requested a review from Copilot June 9, 2026 14:48

Copilot started reviewing on behalf of adriangb June 9, 2026 14:48 View session

Copilot AI reviewed Jun 9, 2026

View reviewed changes

adriangb merged commit 0457ffa into main Jun 9, 2026
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runner: measure benchmark memory and CPU from the process subtree - #14

runner: measure benchmark memory and CPU from the process subtree#14
adriangb merged 3 commits into
mainfrom
metric-scope-bench-process

adriangb commented Jun 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

adriangb commented Jun 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

adriangb commented Jun 9, 2026

Problem

Fix

Background

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

adriangb commented Jun 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants